Skip to content

Fix flaky runWithRateLimit test by using fake timers#7290

Merged
gonzaloriestra merged 1 commit intomainfrom
fix-flaky-rate-limit-test
Apr 14, 2026
Merged

Fix flaky runWithRateLimit test by using fake timers#7290
gonzaloriestra merged 1 commit intomainfrom
fix-flaky-rate-limit-test

Conversation

@gonzaloriestra
Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

Flaky test: https://github.com/Shopify/cli/actions/runs/24396106090

FAIL   @shopify/cli-kit  src/private/node/conf-store.test.ts > runWithRateLimit > throttles the task when the cache is populated recently
AssertionError: expected true to be false // Object.is equality

- Expected
+ Received

- false
+ true

 ❯ src/private/node/conf-store.test.ts:506:19
    504| 
    505|       // Then
    506|       expect(got).toBe(false)
       |                   ^
    507|       expect(taskRan).toBe(false)
    508|     })
 ❯ Module.inTemporaryDirectory src/public/node/fs.ts:81:12
 ❯ src/private/node/conf-store.test.ts:475:5

The 'throttles the task when the cache is populated recently' test was flaky because it used real time with a 1-second timeout window. Under CI load, the time between filling the rate limit cache and checking it could exceed 1 second, causing the old occurrences to be swept out and the rate limit to no longer be exceeded.

WHAT is this pull request doing?

Fix by using vi.useFakeTimers() to freeze time during the test, matching the pattern already used by the 'cache is populated but outdated' test in the same describe block. The existing afterEach hook already calls vi.useRealTimers() for cleanup.

Also applied the same fix to the 'rate limit isn't used up' test to prevent the same class of flake.

How to test your changes?

CI

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing, so I've added a changelog entry with pnpm changeset add

The 'throttles the task when the cache is populated recently' test was
flaky because it used real time with a 1-second timeout window. Under CI
load, the time between filling the rate limit cache and checking it could
exceed 1 second, causing the old occurrences to be swept out and the
rate limit to no longer be exceeded.

Fix by using vi.useFakeTimers() to freeze time during the test, matching
the pattern already used by the 'cache is populated but outdated' test
in the same describe block. The existing afterEach hook already calls
vi.useRealTimers() for cleanup.

Also applied the same fix to the 'rate limit isn't used up' test to
prevent the same class of flake.
@gonzaloriestra gonzaloriestra marked this pull request as ready for review April 14, 2026 12:03
@gonzaloriestra gonzaloriestra requested a review from a team as a code owner April 14, 2026 12:03
@gonzaloriestra gonzaloriestra added this pull request to the merge queue Apr 14, 2026
Merged via the queue into main with commit f833fcf Apr 14, 2026
25 checks passed
@gonzaloriestra gonzaloriestra deleted the fix-flaky-rate-limit-test branch April 14, 2026 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants